Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Automatically install wasm32-unknown-unknown #11

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented Apr 20, 2022

Implements #9

This commit enables automatic installation of the wasm32-unknown-unknown target for the
build and check commands.

The installation will be performed if:

  1. No target param is specified
  2. The wasm32-unknown-unknown target is not already installed
  3. rustup is available as the toolchain installer

This implementation is based on cargo-wasi's install_wasi_target, with the difference that I'm not introducing a cache for the target check.

I opted to leave the cache implementation up for discussion mainly because: (i) the cache can potentially introduce false positives if for example, the user removes the target manually after running build or check for the first time, so if we go ahead with the implementation we might need to think about an "eviction" mechanism analogous to cargo wasi self clean or think of a different caching approach (ii) cargo-wasi has several use-cases for the cache: one being the target installation and the other one being update-checks, perhaps if update checks is something that we want to implement we could add the caching at that time? Right now we'd mostly be saving the rustc --print sysroot invocation. Thoughts?

This commit enables automatic installation of the wasm32-unknown-unknown target for the
`build` and `check` commands.

The installation will be performed if:

1. No `target` param is specified
2. The `wasm32-unknown-unknown` target is not already installed
3. `rustup` is available as the toolchain installer
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good 👍.

Regarding caching the results of the check, I think we can live without that for now but perhaps file an issue to track potentially implementing it if it proves that always having the additional rustc invocation is undesirable (can be avoided by manually specifying the target for now, at least).

@saulecabrera saulecabrera merged commit 74c928a into main Apr 20, 2022
@saulecabrera saulecabrera deleted the install-default-target branch April 20, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants